Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

[v9] Backport Teleport Connect (2 of 5)#763

Merged
ravicious merged 26 commits intoteleport-v9from
ravicious/v9/backport-connect-2-of-5
Apr 27, 2022
Merged

[v9] Backport Teleport Connect (2 of 5)#763
ravicious merged 26 commits intoteleport-v9from
ravicious/v9/backport-connect-2-of-5

Conversation

@ravicious ravicious marked this pull request as ready for review April 26, 2022 11:12
@ravicious ravicious requested a review from kimlisa April 26, 2022 11:16
}, [disconnectAttempt.status]);

useEffect(() => {
if (cluster.connected) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is optional chaining required here? just saw it on line 49

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I look at it, it seems that the checks for cluster being null are unnecessary here. 🤔 If someone logs out from the cluster (and thus removes it) we remove any open resources from that cluster as well. We'll look into removing that after the preview release, thanks!

Comment on lines +26 to +28
css={`
white-space: nowrap;
`}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious between css and style, do both achieve the same thing (just syntax is a bit different?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gzdunek Can you answer Lisa's question here? I remember you were changing that. ;)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

css works the same as styled(Text)... so you have access to the theme for example. Also, css generates a css class, while style adds inline style to the element.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gzdunek i see, thanks! should we be using css from now on? or does it matter?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kimlisa I think we can prefer css over style as it has more capabilities (and nicer syntax) :)

@ravicious ravicious changed the base branch from ravicious/v9/backport-connect-1-of-5 to teleport-v9 April 27, 2022 09:11
@ravicious ravicious changed the base branch from teleport-v9 to ravicious/v9/backport-connect-1-of-5 April 27, 2022 09:14
gzdunek and others added 24 commits April 27, 2022 11:38
This commit removes a bunch of unused protobufs and also updates some of
those that got updated on the teleterm branch in the teleport repo.

The biggest change from all of them is that LoginRequest now has oneof
on Sso and Local. [1] This is because a login request should have either
Sso or Local params, but not both at the same time.

The previous implementation called both `setSso` and `setLogin` on the same
object. This no longer works with the use of `oneof`, because calling `setLogin`
after `setSso` would clear the `Sso` params. [2]

[1] gravitational/teleport#10286 (comment)
[2] https://developers.google.com/protocol-buffers/docs/proto3#oneof_features
@ravicious ravicious force-pushed the ravicious/v9/backport-connect-2-of-5 branch from ebb197b to 5ded89c Compare April 27, 2022 09:41
@ravicious ravicious changed the base branch from ravicious/v9/backport-connect-1-of-5 to teleport-v9 April 27, 2022 09:41
@ravicious ravicious merged commit a550a63 into teleport-v9 Apr 27, 2022
@ravicious ravicious deleted the ravicious/v9/backport-connect-2-of-5 branch April 27, 2022 09:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants